Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add python sys.path to vyper path #3763

Merged
merged 8 commits into from
Feb 13, 2024
Merged

feat: add python sys.path to vyper path #3763

merged 8 commits into from
Feb 13, 2024

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Feb 8, 2024 •
edited
Loading

this allows users to install packages from pip and import them using a regular python workflow.

What I did

opening pandora's box

How I did it

inject sys.path into the vyper compiler's search path

How to verify it

pip install snekmate
cat > test.vy <<EOF
from auth import AccessControl as acl  # AccessControl.vy is in snekmate
EOF
vyper test.vy

Commit message

this makes it easier to install vyper packages from pip and import them
using a regular python workflow.

misc:
- improve how paths appear in error messages; try hard to make them
  relative paths.
- add `chdir_tmp_path` fixture which chdirs to the `tmp_path` fixture
  for the duration of the test.

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@charles-cooper
feat: add python sys path to vyper path
be47d63 
this allows users to install packages from pip and import them using a
regular python workflow.
fubuloubu
fubuloubu previously requested changes Feb 8, 2024
View reviewed changes
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the package name be used as the base of the namespace?

e.g. from snekmate.auth import ...

@charles-cooper
Copy link
Member Author

charles-cooper commented Feb 8, 2024 •
edited
Loading

Can the package name be used as the base of the namespace?

e.g. from snekmate.auth import ...

that's an issue for the package maintainer -- snekmate is currently installing auth to the top- level of site-packages/. better hygiene would be for it to be installed to site-packages/snekmate/ (cc. @pcaversaccio ), then it would be imported as from snekmate.auth import AccessControl.

@fubuloubu fubuloubu dismissed their stale review February 8, 2024 22:06

not valid concern

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a2eb60c) 84.95% compared to head (ee369cc) 69.61%.

❗ Current head ee369cc differs from pull request most recent head 7887ce9. Consider uploading reports for the commit 7887ce9 to get more accurate results

Files Patch % Lines
vyper/cli/vyper_compile.py 0.00% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@             Coverage Diff             @@
##           master    #3763       +/-   ##
===========================================
- Coverage   84.95%   69.61%   -15.34%     
===========================================
  Files          92       92               
  Lines       13595    13135      -460     
  Branches     3061     2935      -126     
===========================================
- Hits        11549     9144     -2405     
- Misses       1560     3319     +1759     
- Partials      486      672      +186

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pcaversaccio
Copy link
Collaborator

pcaversaccio commented Feb 9, 2024 •
edited
Loading

Can the package name be used as the base of the namespace?
e.g. from snekmate.auth import ...

that's an issue for the package maintainer -- snekmate is currently installing auth to the top- level of site-packages/. better hygiene would be for it to be installed to site-packages/snekmate/ (cc. @pcaversaccio ), then it would be imported as from snekmate.auth import AccessControl.

This is correct - I opened a PR to fix this pattern in snekmate: pcaversaccio/snekmate#204 (@charles-cooper can you quickly have a look). If you use the fixed pattern via:

pip install -i https://test.pypi.org/simple/ snekmate==0.0.5rc2

and the introduced changes in this PR, you will get:

Error compiling: Test.vy
vyper.exceptions.ModuleNotFound: vyper.interfaces.ERC165
  contract "/mnt/c/Users/pasca/Desktop/vyper/venvU/lib/python3.11/site-packages/snekmate/auth/AccessControl.vy:54", line 54:0
       53 # which is a built-in interface of the Vyper compiler.
  ---> 54 from vyper.interfaces import ERC165
  --------^
       55 implements: ERC165

The compilation error currently is due to the recent change introduced here #3741 (I would need to refactor this to from ethereum.ercs import ERC165 in the next release), but it works 😄.

If I adjust the snekmate AccessControl.vy module locally to use from ethereum.ercs import ERC165, I however get the following raise:

Error compiling: Test.vy
vyper.exceptions.CompilerPanic: unhandled exception 'ModuleInfo' object has no attribute '_invalid_locations'
  contract "C:\Users\pasca\Desktop\vyper\venv\Lib\site-packages\snekmate\auth\AccessControl.vy:62", line 62:0
       61 import interfaces.IAccessControl as IAccessControl
  ---> 62 implements: IAccessControl
  --------^
       63


This is an unhandled internal compiler error. Please create an issue on Github to notify the developers!
https://github.com/vyperlang/vyper/issues/new?template=bug.md

pcaversaccio
pcaversaccio reviewed Feb 9, 2024
View reviewed changes
Copy link
Collaborator

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add one test case please?

@charles-cooper
Copy link
Member Author

If I adjust the snekmate AccessControl.vy module locally to use from ethereum.ercs import ERC165, I however get the following raise:

seems like an outstanding issue

@charles-cooper charles-cooper added this to the v0.4.0 milestone Feb 10, 2024
fubuloubu
fubuloubu reviewed Feb 12, 2024
View reviewed changes
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figure out a way to mock this for testing

@charles-cooper
Copy link
Member Author

test added here eac2d2f

@charles-cooper charles-cooper changed the title feat: add python sys path to vyper path feat: add python sys.path to vyper path Feb 13, 2024
@charles-cooper
Copy link
Member Author

charles-cooper commented Feb 13, 2024 •
edited
Loading

If I adjust the snekmate AccessControl.vy module locally to use from ethereum.ercs import ERC165, I however get the following raise:

seems like an outstanding issue

that error message should be fixed in #3775

@charles-cooper charles-cooper enabled auto-merge (squash) February 13, 2024 00:46
@charles-cooper charles-cooper merged commit a3bc3eb into vyperlang:master Feb 13, 2024
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcaversaccio pcaversaccio pcaversaccio left review comments

@harkal harkal harkal approved these changes

@fubuloubu fubuloubu fubuloubu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@charles-cooper @codecov-commenter @pcaversaccio @fubuloubu @harkal